-
Notifications
You must be signed in to change notification settings - Fork 394
Accumulate extra pydantic fields from the sample event #1070
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Accumulate extra pydantic fields from the sample event #1070
Conversation
|
Passes locally with Pydantic v2; I added a short-circuit for Pydantic v1 in the test suite since this is not supported. (It looks like CI uses V1.) |
| if event.usage.server_tool_use is not None: | ||
| current_snapshot.usage.server_tool_use = event.usage.server_tool_use | ||
|
|
||
| # Accumulate any extra fields from the event into the snapshot |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: should we just accumulate all the extra fields from events into the message?
TBH, what I really expect here is to see the same extra fields I would get if I made a non-streaming request. Maybe we shouldn't accumulate all the extra kwargs from events, but only the ones that make sense (which are extra kwargs for the non-streaming case too).
- Does that make sense?
- Do we know which events are responsible for transferring the extra fields we would see in the non-streaming case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe non-streaming requests already copy over these extra fields (the code I'm working with broke when we moved from non-streaming to streaming). I'm not sure exactly where that happens, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the confusion! Let me explain what I'm seeing.
So in the non-streaming case, we have extra fields A and B, right? I'd expect the streaming case to have those same fields – no more, no less. But right now it looks like it might be getting more.
It seems like we're accumulating extra fields from all the events into the final message, including stuff from the message-stop event and other events that don't actually hold message data. I don't think that's what we want here – or am I missing something?
Also, could you share the SDK version and the requests you made for both streaming and non-streaming? That would be super helpful for figuring this out!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be enough to gate this logic on the event.type?
I will have to dig up the version numbers and test cases later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be great to understand which particular events are responsible for transferring message extra fields. From the snapshots you've created for the test, I see that these are message_start, content_block_delta, and message_delta. We can potentially update kwargs only if the event is one of these as you mentioned. It would help us avoid filling up the extra kwargs of the final message with unnecessary data.
Also, I noticed that the same kwargs could be sent through different event types. For example, in your snapshot, message_start, content_block_delta, and message_delta are all sending the private_field extra field. Is that intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P.S. I see that message_delta can send some fields which have already been sent by message_start, such as usage or context management fields, so we need to accumulate at least these two events. But I'm not sure about content_block_delta.
Currently undeclared fields are discarded rather than accumulated in
get_final_message; this adds code to deep-merge these fields instead.